Is this the only way to fix it? I would’ve expected us to perhaps do something with end-of-stream rather than selectively ignoring errors.

end-of-stream treats non-ending (destroy) streams that close early as an error condition. I think it is right in doing so.

However this does not play well with async iterators as you pointed out. We had a different logic before that did the same but was less explicit about it: I like this one better.

end-of-stream treats non-ending (destroy) streams that close early as an error condition. I think it is right in doing so.

Can you elaborate? It sounds to me that destroyed streams (with null as error value) should be treated no differently as any other stream that ended. I.e., it's reasonable to believe a stream can be destroyed whenever.

In fact, I would say that the following test case added in #23785 should in fact finish gracefully with an { done: true, value: undefined } rather than throwing an error:

We see precedent for invalidated iterator still operating gracefully in ECMA-262 iterators:

const array = [0, 1, 2, 3];
const iterated = [];
for (const a of array) {
  iterated.push(a);
  if (a === 1) {
    array.splice(0);
  }
  // loop ends immediately.
}
// iterated is [0, 1]

First, end-of-stream is one of the most downloaded utilities on npm, and keeping the API and behavior similar has been a goal. Second, from a consumer perspective a stream that was ended or destroyed is significantly different. That could have easily been a flag as an argument. I think it errors because in the context of pipeline/pump, in fact it is an error: the pipeline did not complete correctly but was interrupted (as an example, gzipping a file).

I’ll check again that test asap, it does not sound correct with this change.

Okay, as long as async iteration isn't affected it works for me.

@TimothyGu the test was botched anyway, and the catch clause was never executed in this case. I've fixed it.